fix(engine): skip video frame injection when a sub-composition host is hidden#1028
fix(engine): skip video frame injection when a sub-composition host is hidden#1028lirian-su-opus wants to merge 5 commits into
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
style-9-prod regression — root cause found
The isVisualAncestorHidden check is too broad. It catches a legitimate rendering scenario in style-9-prod where the runtime's [data-start] lifecycle hides a composition container whose GSAP timeline is shorter than its authored data-duration.
What happens
In style-9-prod, the #aroll-comp element:
- Has
data-composition-id="aroll-layer",data-start="0",data-duration="16.04" - But its GSAP timeline only tweens up to 9.88s
- The runtime truncates visible duration to
Math.min(16.04, 9.88) = 9.88, settingvisibility: hiddenafter t=9.88s
Before this PR: The replacement <img> had visibility: visible which correctly overrides ancestor visibility: hidden (CSS spec: child visibility: visible overrides parent visibility: hidden). The video held its final GSAP state.
After this PR: The ancestor walk finds #aroll-comp with visibility: hidden and skips injection entirely → 38 blank frames from t=9.95s to end (PSNR drops from ~50 to ~11).
Fix
The check should only trigger on sub-composition hosts ([data-composition-src] or [data-composition-file]) — which was the original intent of the PR. Regular [data-start] containers with visibility: hidden are handled correctly by CSS visibility inheritance (the <img>'s explicit visibility: visible overrides the ancestor).
Replace:
const isVisualAncestorHidden = (el: HTMLElement): boolean => {
let parent = el.parentElement;
while (parent !== null && parent !== document.documentElement) {
const computed = window.getComputedStyle(parent);
if (computed.display === "none" || computed.visibility === "hidden") return true;
parent = parent.parentElement;
}
return false;
};With:
const isVisualAncestorHidden = (el: HTMLElement): boolean => {
let parent = el.parentElement;
while (parent !== null && parent !== document.documentElement) {
const computed = window.getComputedStyle(parent);
if (computed.display === "none") return true;
// Only treat visibility:hidden as a skip signal on sub-composition
// hosts. For regular [data-start] containers the replacement <img>'s
// explicit `visibility: visible` correctly overrides the ancestor per
// CSS spec — we must NOT skip injection for those.
if (
computed.visibility === "hidden" &&
(parent.hasAttribute("data-composition-src") || parent.hasAttribute("data-composition-file"))
) return true;
parent = parent.parentElement;
}
return false;
};Both copies of isVisualAncestorHidden (in injectVideoFramesBatch and syncVideoFrameVisibility) need this same change.
`injectVideoFramesBatch` and `syncVideoFrameVisibility` iterate every `video[data-start]` whose raw time window covers the current seek. Inner `<video>` elements inside `[data-composition-src]` sub-compositions get `data-start="0"` auto-injected by `compileTimingAttrs` and probed-duration cover the entire timeline, so they look "active" even when their host has not yet started. When the runtime then hides the host with `visibility: hidden` (its out-of-window lifecycle), the inner video inherits hidden via the CSS cascade — but our injector responded by painting a replacement `<img class="__render_frame__" style="visibility: visible">` next to the video. `visibility: visible` on the descendant defeats the parent `visibility: hidden` cascade, and because the host has not been morphed by GSAP yet the video's bounding box is its CSS default (usually full-bleed). The result is one full-bleed frame per inactive sub-comp painted over whichever moment is *actually* visible — the overlay symptom the upstream agentic-finecut project saw. Walk ancestors in both functions; if any has `display: none` or `visibility: hidden`, skip the inject and hide any stale `__render_frame__` sibling. The render is now correctly empty for hidden hosts, which is what the surrounding CSS cascade already intends. Tests: - `screenshotService.test.ts`: cover the new guard for both visibility:hidden and display:none hosts, both for the fresh-img and the stale-img paths, plus `syncVideoFrameVisibility` for the case where the time window calls a video "active" but a hidden ancestor still requires its frame to stay hidden. Each test fails against pre-fix `screenshotService.ts`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The screenshotService.test.ts regression-suite comment pointed at the author's fork branch as backstory. Strip the line so upstream code doesn't carry a fork-relative reference; the surrounding paragraph already explains the bug end-to-end without it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`isVisualAncestorHidden` was treating any `visibility: hidden` ancestor as a signal to skip injecting the replacement frame. That's too broad — for plain `[data-start]` containers, the replacement `<img>`'s explicit `visibility: visible` correctly overrides the ancestor per CSS spec, and consumers rely on that to hold the final GSAP-driven frame when an authored `data-duration` outlives the composition's GSAP timeline (e.g. `style-9-prod`, where the runtime truncates the host to `visibility: hidden` after the timeline ends and the replacement frame must paint through). Restrict the `visibility: hidden` skip to ancestors that carry `data-composition-src` or `data-composition-file` — the actual sub-composition hosts this guard was added for. `display: none` keeps the broad behavior: it takes the whole subtree out of layout and a child override cannot escape. Update the existing regression suite to mark the host as a sub-composition, and add two new cases pinning the plain-`[data-start]` behavior: both `injectVideoFramesBatch` and `syncVideoFrameVisibility` must still produce a visible replacement `<img>` when the host is `visibility: hidden` but does not carry a sub-composition attribute.
…cache
Two follow-ups to the ancestor-visibility skip in `injectVideoFramesBatch`
and `syncVideoFrameVisibility`.
1. **Mask defence.** Both ancestor-hidden branches previously wrote a plain
`img.style.visibility = "hidden"`. `applyDomLayerMask` writes the
stylesheet rule `#${showId} *{visibility:visible !important}`, and CSS
cascade puts important stylesheet author above non-important inline
author — so a sub-comp host landing in the active layer's `show` set
would revive a stale `__render_frame__` and let it bleed onto the
layer composite. Write the hide via
`style.setProperty("visibility", "hidden", "important")` instead;
important inline beats important stylesheet.
2. **Caller cache hygiene.** `createVideoFrameInjector` unconditionally
wrote `lastInjectedFrameByVideo.set(id, frameIndex)` after calling
`injectVideoFramesBatch`, even for videos the page silently skipped due
to a hidden visual ancestor. On the next call at the same frameIndex —
common with source-fps < output-fps, paused source frames, or
non-frame-aligned host starts — the cache short-circuited the second
inject and the host's first visible frame painted blank because the
replacement `<img>` was never created.
Make `injectVideoFramesBatch` return `string[]` (the subset of ids it
actually painted) and have the caller cache only those. The cli-side
`snapshot.ts` consumer is unaffected: its local `InjectFn` types the
return as `Promise<void>`, which is structurally compatible with
`Promise<string[]>` under TS void-return assignment rules.
Tests: linkedom doesn't preserve `!important` in cssText, so the two new
mask-defence cases spy on the live `<img>`'s `style.setProperty` and assert
the 3-arg call shape. The cache-hygiene case stubs the page-side primitives
via `vi.mock`, drives the hook twice at the same frameIndex with a stubbed
"injected nothing" first response, and verifies the second call still
issues an inject. A counter-test pins the happy-path cache hit so a future
refactor can't trade the skip bug for a never-cache regression.
`injectVideoFramesBatch` now returns `Promise<string[]>` so the caller can
filter cache entries to videos the page actually painted. The cli-side
snapshot command does not use the return value, but its local `InjectFn`
declared `Promise<void>` which made the `as { injectVideoFramesBatch:
InjectFn }` cast on the dynamic engine import fail typecheck under TS's
"sufficiently overlapping types" rule. Match the engine's actual export
shape.
aae9454 to
dcd637b
Compare
What
In
packages/engine/src/services/screenshotService.ts+videoFrameInjector.ts, stop painting the page-side replacement<img class="__render_frame__">when a<video data-start>'s visual ancestor is hidden, and make the skip robust against the two ways it could still cause stale or blank frames:injectVideoFramesBatchandsyncVideoFrameVisibility.display: noneon any ancestor is always a skip signal;visibility: hiddenis a skip signal only on sub-composition hosts ([data-composition-src]/[data-composition-file]).!importantsoapplyDomLayerMask's#${showId} *{visibility:visible !important}rule cannot revive them when a sub-comp host lands in an active layer'sshowset.injectVideoFramesBatch(Promise<string[]>) socreateVideoFrameInjectoronly caches videos the page actually painted — skipped ones must re-attempt next call, not be remembered as up-to-date.Add regression suite covering all three guarantees, plus the carve-out for plain
[data-start]hosts whosevisibility: hiddenis escapable by the descendant<img>'s explicitvisibility: visible.Why
The original symptom — inner-PIP overpaint
The injector iterates every
video[data-start]whose raw[data-start, data-start + data-duration)window covers the current seek and overlays the extracted source-video frame on each one. That contract is wrong for<video>elements nested insidedata-composition-srcsub-compositions:compileTimingAttrsauto-injectsdata-start="0"+ adata-hf-auto-startmarker on any<video>without timing attrs. The duration prober then resolvesdata-endto the full source duration. So an inner PIP<video>covers the entire timeline in the active check, regardless of which moment its host actually belongs to.[data-start]lifecycle hides out-of-window hosts withvisibility: hidden. The cascade hides the nested<video>'s rendered output — but the injector still creates an<img>next to it and explicitly setsimg.style.visibility = "visible", which under CSS rules defeats the parentvisibility: hidden.<video>'s used box is the CSS default — typically full-bleed.Net effect: every inactive sub-comp with a nested
<video>painted one full-bleed replacement frame on top of whichever moment was currently visible.Why the narrowing —
style-9-prodCSS-spec-wise, a descendant
visibility: visibleoverrides an ancestorvisibility: hidden. Regular[data-start]containers rely on that override: instyle-9-prod,#aroll-comphasdata-duration="16.04"but its GSAP timeline only tweens to t=9.88s; the runtime truncates the host tovisibility: hiddenafter t=9.88s, and the replacement<img>must paint through to hold the final-state frame. An unconditional skip dropped 38 tail frames there and crashed PSNR from ~50 to ~11.display: nonedoes not have this carve-out — it removes the subtree from layout entirely and no child override can escape.Why mask defence
applyDomLayerMaskwritesbody *{visibility:hidden !important}plus#${showId} *{visibility:visible !important}for each show id. If a sub-comp host's id lands in the show set, the show rule cascades to descendant__render_frame__siblings. A plain inlinevisibility: hiddenwe wrote in the ancestor-hidden branch loses to important stylesheet author per CSS cascade — the stale frame would re-paint on the layer composite. Inline!importantbeats stylesheet!important(author-origin precedence), which is whatapplyDomLayerMask's ownhidearm already does forextraHideIds.Why cache hygiene
createVideoFrameInjectorrecordedlastInjectedFrameByVideo.set(id, frameIndex)after every call toinjectVideoFramesBatch, including videos the page silently skipped. On the next call at the same sourceframeIndex— common when source fps < output fps, source is paused, or host start isn't frame-aligned — the cache short-circuited the second inject and the host's first visible frame painted blank because the<img>was never created. Returning the actually-painted ids and caching only those keeps the skipped videos re-eligible for injection on the next tick.How
Ancestor check (both functions)
Walk starts at
parentElementbecause both functions writevisibility: hidden !importantto the<video>itself, so reading the video's own computed visibility is unreliable.injectVideoFramesBatch— short-circuit per video: hide a stale__render_frame__sibling viasetProperty("visibility", "hidden", "important"), thencontinue. Do not push the videoId intoinjectedIds. ReturninjectedIdsto the caller at the end.syncVideoFrameVisibility— fall through into the inactive arm so the<img>lands on inline!importanthidden, matching what CSS already implies for the rest of the subtree.The helper is inlined in both functions instead of hoisted to a shared module because the bodies run inside
page.evaluateand a cross-file string-serialized helper would be more brittle than two identical copies. The regression tests exercise both paths so drift would surface.Caller cache
Alternatives considered and rejected
snapshot.ts,producer/services/videoFrameInjector.ts). Two equivalent implementations would have to stay in sync, and any future caller would have to remember to filter.data-start/data-endand recompute active per-video. Couples the injector to the timing convention; doesn't generalize to hosts hidden for other authoring reasons.<video>elements when the host is out of window. Same outcome but a much bigger blast radius — the injector is the layer that knows it's about to override the cascade, so it's the natural enforcement point.page.evaluateper tick wheninjectVideoFramesBatchalready has the DOM in scope and can return the same information for free.Test plan
screenshotService.test.ts—video-frame injection respects ancestor visibility(8 cases)visibility: hiddensub-comp host →injectVideoFramesBatchcreates no<img>next to the video.display: nonesub-comp host → same.__render_frame__<img>+ sub-comp host nowvisibility: hidden→injectVideoFramesBatchflips it tovisibility: hidden.syncVideoFrameVisibilitycalled with the video inactivebut a sub-comp ancestor hidden → the existing<img>stays hidden, not "visible".style-9-prodregression guard: plain[data-start]host atvisibility: hidden→injectVideoFramesBatchstill injects the<img>withvisibility: visible.style-9-prodregression guard (sync arm): plain[data-start]host atvisibility: hidden, video active →syncVideoFrameVisibilityflips an existing<img>tovisibility: visible.<img>→injectVideoFramesBatchcallsstyle.setProperty("visibility", "hidden", "important")soapplyDomLayerMask's important stylesheet rule cannot revive it. Asserted via spy on the live<img>'ssetProperty(linkedom strips!importantfrom cssText/getPropertyPriority).syncVideoFrameVisibility→ same spy assertion.setupHostHiddenScenariotakes an optionalhostAttributeso cases 5/6 swap the host'sdata-composition-srcfordata-start.videoFrameInjector.test.ts—createVideoFrameInjector cache hygiene against page-side skips(2 cases)[](ancestor-hidden), second call at the sameframeIndexmust still issue an inject.["pip"], second call at the sameframeIndexshort-circuits (no second inject). Pin so a future refactor can't trade the skip bug for a never-cache regression.Stubs are wired via
vi.mock("./screenshotService.js", ...)with hoisted spies oninjectVideoFramesBatchandsyncVideoFrameVisibility; theFrameLookupTableis faked with a one-payloadgetActiveFramePayloads, andframeSrcResolvershort-circuits the on-disk cache.Suite totals
bun run --filter @hyperframes/engine test:619/622 passed. The three failures are inffprobe.test.ts(HDR PNG cICP metadata parsing + ffprobe-missing fallback) and reproduce identically onmain— unrelated to this change.hyperframes renderrun, confirmed the visible host's content shows through after the fix; verifiedbunx oxlint/bunx oxfmt --checkand the engine test suite minus the pre-existing failures)